Skip to content

migrate contract e2e#2716

Draft
open-junius wants to merge 12 commits into
devnet-readyfrom
migrate-contract-e2e
Draft

migrate contract e2e#2716
open-junius wants to merge 12 commits into
devnet-readyfrom
migrate-contract-e2e

Conversation

@open-junius
Copy link
Copy Markdown
Contributor

Description

Migrate contract e2e to new framework.

Related Issue(s)

  • Closes #[issue number]

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Other (please describe):

Breaking Change

If this PR introduces a breaking change, please provide a detailed description of the impact and the migration path for existing applications.

Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have run ./scripts/fix_rust.sh to ensure my code is formatted and linted correctly
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Screenshots (if applicable)

Please include any relevant screenshots or GIFs that demonstrate the changes made.

Additional Notes

Please provide any additional information or context that may be helpful for reviewers.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 4, 2026

🛡️ AI Review — Skeptic (security review)

VERDICT: SAFE

BASELINE scrutiny: open-junius has repo write permission and substantial prior subtensor history; branch targets devnet-ready; Gittensor allowlist search had no hit.

Static review covered the trusted instructions, cached PR metadata, contributor signals, prior Skeptic output, branch target, changed-file list, and full diff. The PR does not modify .github/ai-review/* or .github/copilot-instructions.md; the workflow change only adds a zombienet_evm matrix entry under an existing pull_request workflow with contents: read. No new dependency or lockfile changes, build-script changes, secret handling, CI permission expansion, or runtime behavior changes were found.

Findings

No findings.

Prior-comment reconciliation

  • 5beb761f: no longer applies — The current diff against devnet-ready does not remove or change the sender-side staking validation path; pallets/subtensor/src/staking/order_swap.rs only removes a blank line.

Conclusion

No malicious intent or PR-introduced security vulnerability was found in the current diff. The only runtime-file change is a blank-line removal, and the rest of the PR migrates existing EVM transfer coverage into the TypeScript/zombienet test framework.


🔍 AI Review — Auditor (domain review)

VERDICT: 👍

LIKELY gittensor; open-junius has write permission and many recent merged subtensor PRs, but is not present in the trusted allowlists.

The Auditor proposed a replacement PR description, but the current body is non-trivial; not overwriting. Maintainers: ask the Auditor to regenerate if you want it.

Static review only. I did not run the Moonwall suite because this workspace has no pnpm or installed ts-tests/node_modules.

No runtime behavior changed: the only pallet edit is a blank-line removal in pallets/subtensor/src/staking/order_swap.rs, so no spec_version bump is required. git diff --check passes.

Overlapping PRs were checked. #2718 overlaps in contract E2E files but is a broader contracts-E2E optimization branch, while this PR is the focused Moonwall migration for the EVM transfer suite; this PR remains the better candidate for this migration. The other overlaps are incidental workflow, governance, pallet/runtime, or transaction-payment changes rather than duplicate implementations.

Findings

Sev File Finding
LOW ts-tests/pnpm-workspace.yaml:28 Resolve allowBuilds placeholders inline

Prior-comment reconciliation

  • dac5f10e: not addressed — The literal allowBuilds placeholder values are still present in ts-tests/pnpm-workspace.yaml; carried forward as the current low-severity finding.

Conclusion

Approving with one carried-forward low-severity cleanup still outstanding. The EVM transfer E2E coverage is migrated into the TypeScript/Moonwall CI path, and the remaining issue is limited to unresolved pnpm policy placeholders.


📜 Previous run (superseded)
Sev File Finding Status
LOW ts-tests/pnpm-workspace.yaml:28 Resolve allowBuilds placeholders ➡️ Carried forward to current findings
The literal allowBuilds placeholder values are still present in ts-tests/pnpm-workspace.yaml; carried forward as the current low-severity finding.

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

}
]
}, {
"name": "zombienet_evm",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] Wire the new EVM suite into CI

This registers a new zombienet_evm Moonwall suite, but .github/workflows/typescript-e2e.yml still only runs dev, zombienet_shield, zombienet_staking, zombienet_coldkey_swap, and zombienet_subnets. As written, the migrated EVM test can go stale because PR CI never invokes pnpm moonwall test zombienet_evm.

Add zombienet_evm to the run-e2e-tests matrix, likely against the release binary unless there is a specific fast-runtime reason.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 4, 2026

🔄 AI review updated — Skeptic: SAFE Auditor: 👎

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

Comment on lines +20 to +28
allowBuilds:
'@biomejs/biome': set this to true or false
'@parcel/watcher': set this to true or false
cpu-features: set this to true or false
esbuild: set this to true or false
msgpackr-extract: set this to true or false
protobufjs: set this to true or false
sqlite3: set this to true or false
ssh2: set this to true or false
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] Resolve allowBuilds placeholders

The new allowBuilds map still contains generated placeholder strings instead of explicit boolean decisions. pnpm 10.26+ defines this field as a map that explicitly allows (true) or disallows (false) dependency build scripts, so leaving set this to true or false here keeps the new policy unresolved and can cause install-time warnings or stricter future failures. (newreleases.io)

Suggested change
allowBuilds:
'@biomejs/biome': set this to true or false
'@parcel/watcher': set this to true or false
cpu-features: set this to true or false
esbuild: set this to true or false
msgpackr-extract: set this to true or false
protobufjs: set this to true or false
sqlite3: set this to true or false
ssh2: set this to true or false
allowBuilds:
'@biomejs/biome': true
'@parcel/watcher': true
cpu-features: true
esbuild: true
msgpackr-extract: true
protobufjs: true
sqlite3: true
ssh2: true

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 4, 2026

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

Comment on lines +20 to +28
allowBuilds:
'@biomejs/biome': set this to true or false
'@parcel/watcher': set this to true or false
cpu-features: set this to true or false
esbuild: set this to true or false
msgpackr-extract: set this to true or false
protobufjs: set this to true or false
sqlite3: set this to true or false
ssh2: set this to true or false
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] Resolve allowBuilds placeholders

This allowBuilds block still contains generated placeholder values instead of concrete booleans. Since the same packages are already listed under onlyBuiltDependencies, leaving these placeholder strings makes the workspace policy ambiguous and can confuse future pnpm maintenance. Either remove the unfinished block or replace each entry with intentional true/false values.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 4, 2026

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

Comment on lines +20 to +28
allowBuilds:
'@biomejs/biome': set this to true or false
'@parcel/watcher': set this to true or false
cpu-features: set this to true or false
esbuild: set this to true or false
msgpackr-extract: set this to true or false
protobufjs: set this to true or false
sqlite3: set this to true or false
ssh2: set this to true or false
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] Resolve allowBuilds placeholders

The allowBuilds entries are still literal placeholder strings (set this to true or false). Resolve this before merge by either replacing them with explicit boolean policy values or removing the block if onlyBuiltDependencies above is the intended source of truth. Leaving generated placeholders makes the install policy ambiguous and can confuse future pnpm approve-builds or dependency review work.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 5, 2026

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 5, 2026

🔄 AI review updated — Skeptic: SAFE

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

Error::<T>::AmountTooLow
);
Self::ensure_stake_operation_limit_not_exceeded(from_hotkey, from_coldkey, netuid)?;
Self::ensure_available_to_unstake(from_coldkey, netuid, amount)?;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] Restore sender staking rate-limit validation

transfer_staked_alpha(..., validate_sender: true, ...) is the path pallets/limit-orders uses when collecting staked alpha from sell-order signers, and that call site explicitly relies on validate_sender to check the user's rate limit. The trait contract also says this flag must reject a sender that already staked in the current block. This hunk removes the only sender-side stake-operation-limit check and leaves only ensure_available_to_unstake, so a signer can stake and then transfer that same-block stake into order matching, bypassing the intended anti-same-block staking guard. Restore an equivalent rate-limit check before the availability check; the removed helper name may need to be reintroduced or replaced with the current rate-limit API.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 5, 2026

🔄 AI review updated — Skeptic: VULNERABLE

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

Error::<T>::AmountTooLow
);
Self::ensure_stake_operation_limit_not_exceeded(from_hotkey, from_coldkey, netuid)?;
Self::ensure_available_to_unstake(from_coldkey, netuid, amount)?;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] Restore sender staking rate-limit validation

collect_assets in pallets/limit-orders calls this with validate_sender: true and documents that this path should check the seller's staking rate limit before pulling staked alpha into the pallet intermediary. The current implementation now validates hotkey existence, minimum amount, and locks, but no longer rejects a sender that is still inside the staking operation limit window before debiting stake. That lets repeated sell-side batch executions bypass the economic throttling that ordinary stake movement is supposed to enforce. Restore the sender-side staking rate-limit check before ensure_available_to_unstake.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 5, 2026

🔄 AI review updated — Skeptic: VULNERABLE

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

Comment on lines +21 to +28
'@biomejs/biome': set this to true or false
'@parcel/watcher': set this to true or false
cpu-features: set this to true or false
esbuild: set this to true or false
msgpackr-extract: set this to true or false
protobufjs: set this to true or false
sqlite3: set this to true or false
ssh2: set this to true or false
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] Resolve allowBuilds placeholders

These entries are still the literal placeholder strings from pnpm approve-builds. Please replace each value with an explicit boolean policy, or remove the allowBuilds block if onlyBuiltDependencies is the intended policy source. Leaving placeholders makes the workspace build policy ambiguous and carries forward the prior review finding.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 5, 2026

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

Comment on lines +21 to +28
'@biomejs/biome': set this to true or false
'@parcel/watcher': set this to true or false
cpu-features: set this to true or false
esbuild: set this to true or false
msgpackr-extract: set this to true or false
protobufjs: set this to true or false
sqlite3: set this to true or false
ssh2: set this to true or false
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] Resolve allowBuilds placeholders

These literal placeholder strings are still committed under allowBuilds. pnpm expects concrete boolean approvals here; leaving set this to true or false makes the build-script policy ambiguous and can break strict install/config validation. Since these packages are already listed in onlyBuiltDependencies, set the corresponding approvals explicitly.

Suggested change
'@biomejs/biome': set this to true or false
'@parcel/watcher': set this to true or false
cpu-features: set this to true or false
esbuild: set this to true or false
msgpackr-extract: set this to true or false
protobufjs: set this to true or false
sqlite3: set this to true or false
ssh2: set this to true or false
'@biomejs/biome': true
'@parcel/watcher': true
cpu-features: true
esbuild: true
msgpackr-extract: true
protobufjs: true
sqlite3: true
ssh2: true

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 5, 2026

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant